fix(agent-mesh): make MerkleAuditChain root reproducible by an independent rebuild - #3361
fix(agent-mesh): make MerkleAuditChain root reproducible by an independent rebuild#3361chopmob-cloud wants to merge 4 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
🤖 AI Agent: docs-sync-checker — Docs Sync
Docs SyncDocumentation is in sync. |
🤖 AI Agent: contributor-guide — What you did well:
Welcome, and thank you for your detailed contribution! It's great to see the thorough analysis and testing you've included. What you did well:Your fix is well-documented, with clear reasoning and robust test coverage for reproducibility and tamper detection. Actionable items:
For more details on contributing, please refer to our CONTRIBUTING.md. |
🤖 AI Agent: breaking-change-detector — API Compatibility
API Compatibility
|
🤖 AI Agent: code-reviewer — View details
TL;DR: 0 blockers, 1 warning. The fix improves correctness and reproducibility without introducing security risks.
Action items: None. Warnings:
|
🤖 AI Agent: test-generator — `agent-mesh/src/agentmesh/governance/audit.py`
|
🤖 AI Agent: security-scanner — View details
No security issues found. |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
|
🔴 Contributor Check: HIGH
Automated check by AGT Contributor Check. |
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- Replace full-rebuild-per-append with a root-equivalent incremental fix. Measured: main appends 5000 entries in 0.24s; this PR takes 87.6s (~365x, O(n^2)). ADR-0017 requires sub-millisecond per-entry overhead and AUDIT-COMPLIANCE-1.0.md section 9.3 says the tree MUST be built incrementally. Preferred ~5-line fix: pad interior level k with the empty-subtree constant E(k) where E(0)='0'*64, E(k+1)=SHA-256(E(k)||E(k)) — both constructions then converge to the canonical root with O(log n) appends. Alternative: dirty-flag lazy rebuild in get_root_hash/get_proof.
- test_incremental_root_matches_rebuild_for_every_size is tautological after the fix: _rebuilt_root() calls chain._rebuild_tree(), but add_entry now IS _rebuild_tree, so the test compares the function with itself and can never fail. Inline a genuinely independent pure-hashlib textbook recomputation instead. Also add the empty-chain case (root is None).
- Add a BREAKING_CHANGES.md entry: exported merkle_root values change for most chain sizes (22 of the first 32) vs prior releases; consumers who archived export() output for compliance evidence will see mismatches on re-verification.
- (follow-up, non-blocking) pin the canonical construction in AUDIT-COMPLIANCE-1.0.md 9.3; consider RFC-6962-style leaf/interior domain separation while roots are already changing; delete the now-dead odd-duplication branch in _rebuild_tree.
Minor:
- The bug itself is real and precisely diagnosed — verified empirically: main diverges from an independent textbook recomputation at n=5,6,9-14,17-30; the fix diverges nowhere. Severity honestly classified (over-rejection of honest logs, not tamper concealment).
…t (review) Address review on microsoft#3361: replace the O(n^2) rebuild-per-append with an O(log n) incremental fix, per ADR-0017 and AUDIT-COMPLIANCE-1.0.md 9.3. - add_entry pads interior level k with the empty-subtree constant E(k) (E(0)='0'*64, E(k+1)=SHA-256(E(k)||E(k))) instead of the leaf sentinel, so the incremental root converges on the same canonical root as a from-scratch rebuild while staying O(log n) per append (5000 appends in ~0.3s vs ~88s for the rebuild approach). - Tests recompute the root with an independent pure-hashlib textbook Merkle instead of comparing _rebuild_tree with itself; add the empty-chain case and a separate incremental-vs-rebuild cross-check. - Remove the now-dead odd-duplication branch in _rebuild_tree. - Document the exported-root change in agent-mesh/CHANGELOG.md and BREAKING_CHANGES.md.
|
Thanks, this is exactly right on all points. Pushed the preferred incremental fix. E(k) incremental (no more full rebuild). Tests are no longer tautological. The reproducibility test now recomputes the root with an independent pure-hashlib textbook Merkle that shares no code with Docs. Added a Also done. Removed the now-dead odd-duplication branch in Verification: On the non-blocking follow-ups: happy to pin the canonical construction in AUDIT-COMPLIANCE-1.0.md 9.3 and to add RFC-6962-style leaf/interior domain separation while roots are already changing, if you would like those in this PR or a follow-up. Thanks again for the careful review and the independent reproduction. |
|
Thanks for the review, and for pinning the divergence down to the exact sizes. All four points are addressed on the branch (commit a438ab4); each maps to a change below.
CI is green. The RFC-6962 leaf/interior domain separation and the AUDIT-COMPLIANCE 9.3 pinning I have left as the follow-ups you flagged, to keep this PR to the root-reproducibility fix. Re-requesting review when you have a moment. |
…ndent rebuild MerkleAuditChain built its tree two ways that were meant to agree but did not. add_entry updated the tree incrementally and padded interior levels with singleton zero nodes, while _rebuild_tree (the documented verification path) pads at the leaf level and hashes the zero padding upward. The two roots diverged once a real entry's authentication path reached an interior padding node, first at five entries and again at 6, 9, 10 and most non power of two sizes. Because export() emits the incremental root, a verifier that rebuilds a standard Merkle tree from the exported entries computed a different root and rejected an untampered log. Rebuild the tree from all leaves on each append so the recorded root is byte identical to an independent recomputation. Add regression tests covering root reproducibility, inclusion proof verification, and tamper detection. Signed-off-by: chopmob-cloud <250041792+chopmob-cloud@users.noreply.github.com>
…t (review) Address review on microsoft#3361: replace the O(n^2) rebuild-per-append with an O(log n) incremental fix, per ADR-0017 and AUDIT-COMPLIANCE-1.0.md 9.3. - add_entry pads interior level k with the empty-subtree constant E(k) (E(0)='0'*64, E(k+1)=SHA-256(E(k)||E(k))) instead of the leaf sentinel, so the incremental root converges on the same canonical root as a from-scratch rebuild while staying O(log n) per append (5000 appends in ~0.3s vs ~88s for the rebuild approach). - Tests recompute the root with an independent pure-hashlib textbook Merkle instead of comparing _rebuild_tree with itself; add the empty-chain case and a separate incremental-vs-rebuild cross-check. - Remove the now-dead odd-duplication branch in _rebuild_tree. - Document the exported-root change in agent-mesh/CHANGELOG.md and BREAKING_CHANGES.md. Signed-off-by: chopmob-cloud <250041792+chopmob-cloud@users.noreply.github.com>
…t (review) Address review on microsoft#3361: replace the O(n^2) rebuild-per-append with an O(log n) incremental fix, per ADR-0017 and AUDIT-COMPLIANCE-1.0.md 9.3. - add_entry pads interior level k with the empty-subtree constant E(k) (E(0)='0'*64, E(k+1)=SHA-256(E(k)||E(k))) instead of the leaf sentinel, so the incremental root converges on the same canonical root as a from-scratch rebuild while staying O(log n) per append (5000 appends in ~0.3s vs ~88s for the rebuild approach). - Tests recompute the root with an independent pure-hashlib textbook Merkle instead of comparing _rebuild_tree with itself; add the empty-chain case and a separate incremental-vs-rebuild cross-check. - Remove the now-dead odd-duplication branch in _rebuild_tree. - Document the exported-root change in agent-mesh/CHANGELOG.md and BREAKING_CHANGES.md.
a1ded3c to
55921b4
Compare
There was a problem hiding this comment.
Pull request overview
Fixes Merkle root reproducibility in agentmesh.governance.audit by making the incremental Merkle construction use canonical empty-subtree hashes so exported merkle_root values can be independently recomputed.
TL;DR: 2 blockers, 2 warnings. Fix #1 and #2 and this ships.
| # | Sev | Issue | Where |
|---|---|---|---|
| 1 | Block | assert … is True boolean comparison in new test |
test_merkle_audit_root_reproducible.py |
| 2 | Block | PR description “Fix” section describes a different approach (full rebuild O(n) per append) than the code implements | BREAKING_CHANGES.md (anchor for discrepancy) |
| 3 | Warn | Complexity wording says “O(log n) per append” but growth steps are O(n); should say amortized | audit.py docstring |
| 4 | Warn | Same “O(log n) per append” wording in release notes | BREAKING_CHANGES.md, CHANGELOG.md |
Changes:
- Introduce canonical empty-subtree hashing
E(k)and use it for interior padding during incremental growth. - Simplify
_rebuild_treepairing logic by relying on power-of-two leaf padding. - Add regression tests plus changelog/breaking-change entries describing the behavior change.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
agent-governance-python/agent-mesh/src/agentmesh/governance/audit.py |
Adds E(k) empty-subtree padding to keep incremental roots reproducible vs rebuilds. |
agent-governance-python/agent-mesh/tests/test_merkle_audit_root_reproducible.py |
Adds reproducibility, inclusion-proof, and tamper-detection regression tests. |
agent-governance-python/agent-mesh/CHANGELOG.md |
Documents the reproducible Merkle root fix in Unreleased notes. |
BREAKING_CHANGES.md |
Notes root/proof value changes for affected chain sizes and migration guidance. |
Comments suppressed due to low confidence (1)
BREAKING_CHANGES.md:28
- This entry says the update "stays O(log n) per append", but the implementation still performs O(n) work on capacity-doubling steps (padding every level). Consider wording this as amortized O(log n) to avoid implying a strict per-append worst-case bound.
reproduced by a verifier that rebuilt the tree. Interior padding now uses `E(k)`,
so the incremental root, a from-scratch rebuild, and an independent textbook
recomputation all agree, and the update stays O(log n) per append.
| proof = chain.get_proof(entry.entry_id) | ||
| assert proof is not None | ||
| assert chain.verify_proof(entry.entry_hash, proof, root) is True |
| incremental construction converges on the same canonical root as | ||
| :meth:`_rebuild_tree`, so an exported ``merkle_root`` is reproducible by a | ||
| verifier, while the update stays O(log n) per append. |
| and an exported `merkle_root` could not be reproduced by an independent | ||
| verifier. Interior padding now uses `E(k)`, keeping the update O(log n) per | ||
| append. Exported `merkle_root` values change for the affected sizes; see |
| **What changed:** | ||
|
|
||
| The incremental tree builder padded interior levels with the leaf sentinel | ||
| `'0' * 64` rather than the empty-subtree constant `E(k)` (where `E(0) = '0' * 64` | ||
| and `E(k+1) = SHA-256(E(k) || E(k))`). The recorded root therefore diverged from | ||
| a from-scratch rebuild of the same leaves for most chain sizes (22 of the first | ||
| 32: n = 5, 6, 9-14, 17-30, and so on), so an exported `merkle_root` could not be | ||
| reproduced by a verifier that rebuilt the tree. Interior padding now uses `E(k)`, | ||
| so the incremental root, a from-scratch rebuild, and an independent textbook | ||
| recomputation all agree, and the update stays O(log n) per append. |
…st hygiene The changelog, breaking-changes note and add_entry docstring said the update is O(log n) per append; capacity-doubling does O(n) padding work, so state it as amortized O(log n) (worst-case O(n) at growth). Drop an 'is True' comparison and record why test_incremental_root_matches_full_rebuild calls the private _rebuild_tree deliberately. No behaviour change. Signed-off-by: chopmob-cloud <250041792+chopmob-cloud@users.noreply.github.com>
| fresh = MerkleAuditChain() | ||
| for entry in list(chain._entries): | ||
| fresh.add_entry(entry) | ||
| assert fresh.get_root_hash() == recorded |
… chain add_entry rewrites previous_hash/entry_hash in place, so passing the original AuditEntry objects into the fresh chain mutated the entries still held by the source chain, leaving the sibling independent-recompute assertion no longer independent. Deep-copy each entry so the two chains share no mutable state. Signed-off-by: iLoveChicken <chopmob@gmail.com>
2fea630 to
6034db7
Compare
|
Thanks for the detailed review, and apologies for the slow turnaround. All three blocking points are addressed in the current revision; summarising what changed against each: 1. O(n²) rebuild → incremental E(k) padding. As a rough sanity check I just benchmarked it locally — 5000 appends in ~1.1s here, about 0.2 ms/entry. That's on different hardware from your measurements so it isn't a like-for-like comparison with your numbers; I'm only citing it to show the O(n²) blow-up is gone and it's comfortably under the ADR-0017 sub-millisecond-per-entry target. Correctness is covered by the reproducibility tests below rather than by timing. 2. Tautological test replaced. The 3. BREAKING_CHANGES.md entry added. Documents that the exported Copilot's inline notes are also handled: the complexity is documented as amortized O(log n) in the docstring, CHANGELOG, and BREAKING_CHANGES; and the shared-object mutation it flagged is fixed by deep-copying entries in On the non-blocking follow-ups: I've left the AUDIT-COMPLIANCE 9.3 canonical-construction pin and the RFC-6962 domain-separation idea as separate future work to keep this PR scoped to the reproducibility fix — happy to open a follow-up issue for them. Could you take another look when you have a moment? |
Summary
MerkleAuditChaininagent-mesh/src/agentmesh/governance/audit.pybuilds its tamper-evidence Merkle tree two ways that are meant to agree but do not, so the exportedmerkle_rootis not reproducible by an independent verifier for most chain sizes.Root cause
The class has two constructions:
add_entryupdates the tree incrementally. When it grows capacity it pads interior tree levels with singleton zero nodes (MerkleNode(hash="0" * 64))._rebuild_tree(docstring: "full rebuild, used for verification") pads at the leaf level and hashes the zero padding upward, so an interior empty subtree issha256(ZERO + ZERO)..., not"0" * 64.The two roots agree while every interior node on a real leaf's authentication path is itself real, but diverge as soon as that path reaches an interior padding node. That first happens at five entries, and again at 6, 9, 10 and most non power of two sizes.
AuditLog.export()emits the incremental root asmerkle_root. A third party (or the module's own_rebuild_tree) that recomputes a standard Merkle tree from the exported entries gets a different root and rejects a genuine, untampered log. This is a correctness and reproducibility defect in the integrity tooling; it does not let an attacker conceal tampering (the effect is over rejection of honest logs, and the liveverify_integritypath uses the linearprevious_hashchain, which is unaffected).Reproduction
On a clean install of the shipped package, comparing
get_root_hash()against an independent textbook Merkle recomputation of the same leaf hashes:Fix
Pad each interior tree level with the empty-subtree constant
E(k)instead of the leaf sentinel'0' * 64, whereE(0) = '0' * 64andE(k+1) = SHA-256(E(k) || E(k)). That is the value a from-scratch rebuild produces for an all-zero subtree of heightk, so the incrementaladd_entryroot, the module's own_rebuild_tree, and an independent textbook recomputation all converge on the same canonical root for every entry count. The empty-subtree constants are grown lazily (_empty_subtree_hash), so the update stays amortizedO(log n)per append (worst caseO(n)only when tree capacity doubles) — this preserves the incremental construction rather than switching to a full rebuild.(An earlier revision of this PR did a full
_rebuild_treeon everyadd_entry; that wasO(n)per append /O(n^2)over a chain and is replaced by theE(k)padding above, which keeps the per-append cost of the original incremental design.)Validation
Clean Python 3.12 venv, package installed from source at this PR's head.
test_merkle_audit_root_reproducible.py: all reproducibility tests pass.MerkleAuditChain): agree for everyn = 1..128— across seven capacity doublings and every size that diverged before the fix.O(log n)(5000 entries in ~0.6 s), rather than the quadratic cost of a per-append full rebuild.New tests in
test_merkle_audit_root_reproducible.pycover incremental vs independent-recompute agreement across several capacity doublings, incremental vs_rebuild_treeagreement, the five entry minimal reproducer, inclusion proof verification against the recorded root, and tamper detection.Note on classification
I read this as a correctness bug rather than an exploitable vulnerability, since it causes honest logs to fail verification rather than letting tampering pass, and no trust boundary is crossed. If maintainers see a security angle I have missed, I am happy to move the discussion to a private channel.